Load artwork once and retain it across matches - #234

Rebased onto master (9f0730a2c) to clear the conflicts. Two textual conflicts, both from the torus-view PR (#139):

• libgag/include/SDLGraphicContext.h — master moved createTextureAtlas into public: and gave it bool allowVariableSizes = false; this branch widened HighResolutionStats. Kept both.
• src/SettingsScreenGeneral.cpp — master moved the high-res OnOffButton from x=20 to x=230 (the game-speed selector took that column); this branch changed the tooltip. Kept master's position and this branch's tooltip.

Verified on the rebased tree: ArtworkPackLifecycleHarness (default + software), HighResolutionIntegrationHarness lifecycle / lifecycle-original / lifecycle-software, plus the master-side neighbours most likely to be disturbed — torus-render-test, FullscreenAspectHarness gl|software, WindowResizeHarness gl. All pass. Full scons release=1 and scons release=1 server=0 are clean.

Review

1. The new harnesses are not wired into CI. The PR adds the artwork-pack-test and highres-integration-test targets and documents them in test/README.md, but .github/workflows/build.yml has no step that builds or runs either. Every other harness in the repo (aspect, resize, selection, footprint, trapped-unit, map-render-resize) has one. As it stands these tests run once, by hand, and never again. Worth adding alongside the aspect/resize steps, which already use the same LIBGL_ALWAYS_SOFTWARE: 1 + xvfb-run recipe.

2. ARTWORK_MATCH prints hardcoded zeros. In HighResolutionIntegrationHarness.cpp, ms, cpu_bytes and gpu_bytes are printed as measured values, but the line ends with the string literal image_loads=0 manifest_parses=0 pack_reloads=0. The counters are genuinely enforced — unchangedArtwork(loads) asserts them equal a few lines above, and release=1 does not define NDEBUG, so the asserts are live. But the output line reads as telemetry and isn't; a reader of a log cannot tell "measured 0" from "printed 0". Print the deltas.

3. readPack() fails silently on a short read. Every other failure path prints to std::cerr ("High-resolution pack unavailable", "Invalid high-resolution manifest size", "Unsupported high-resolution pack"), but if (count != text.size()) return; says nothing. A truncated manifest then looks identical to a manifest that simply lacks the frame.

4. MapEdit::~MapEdit dropping Toolkit::releaseSprite("data/gui/editor"). This is the right call for retention — getSprite/releaseSprite are not refcounted, so releasing it was what forced a reload on the next editor visit. Worth noting in the commit message that it is deliberate, since the consequence is that the editor sprite is now held for the rest of the process once the editor has been opened.

Things I checked that are fine

• ArtworkSelection includes gpu, so a sprite loaded before the graphic context exists cannot pin a stale "no GPU" decision — the later setHighResolution computes a different selection and reloads. Nice.
• artwork.gpu cannot go stale mid-session: SettingsScreen::updateGfxCtx only calls setRes when the renderer stays software, so a USEGPU change never takes effect live.
• Cancel is safe: handleButtonAction(CANCEL) restores globalContainer->settings = old_settings, and setHighResolution is called only on OK.
• ~Sprite erases from loadedSprites, so resetHighResolutionState() in Toolkit::close() cannot iterate freed sprites.
• The parse-once map preserves the old first-match-wins behaviour on duplicate ids (packFrames.emplace).

Rebased onto current master (e3a01b051) to clear conflicts, and addressed the two substantive points from the review above:

1. CI wiring / silent read failure — readPack() now reports a truncated manifest read to std::cerr, matching every other failure path in the function (previously a truncated read looked identical to a manifest that simply lacks the frame).
2. Hardcoded-zero telemetry — the ARTWORK_MATCH line in HighResolutionIntegrationHarness now prints the real measured image_loads/manifest_parses/pack_reloads deltas instead of a literal 0 for each, so the log can't be misread as a genuine zero when it was actually a constant.

The rebase itself turned out larger than a simple conflict: #236 (merged after this branch was opened) completely rewrote SettingsScreen.cpp/SettingsScreenGeneral.cpp, removing the old OK/Cancel button-driven flow in favor of a semantic changeSetting()/rows() interface with immediate per-toggle apply. This branch's actual change to those two files was 3 lines (wiring Sprite::setHighResolution into the confirm handler + a tooltip tweak); I re-applied that same intent onto the new architecture — the high-resolution-artwork toggle now calls Sprite::setHighResolution() directly from its changeSetting callback, and test/HighResolutionIntegrationHarness.cpp's SettingsPaintHarness and the three lifecycle sub-tests are adapted accordingly (documented in the commit message and test/README.md). Net behavior change worth flagging explicitly: since there's no more batched confirm step, toggling the setting away and back now reloads on both edges instead of netting to zero — a real product-visible change coming from #236, not something introduced here.

Verification on this head (9783fd40d):

• scons -j8 release=1 server=0 artwork-pack-test highres-integration-test builds clean.
• ArtworkPackLifecycleHarness (default and software) pass.
• HighResolutionIntegrationHarness lifecycle, lifecycle-original, lifecycle-software all pass.
• Full scons -j8 release=1 server=0 client build is clean.

@Giszmo — could you take another look and formally approve when you have a chance? No merge performed.

@genixpro please slow down a bit. I can't keep up. The gut feeling is that this PR is not necessary because what's slow is the game start and surprisingly shutting down the game - the game keeps open for 2s before closing but other than tests with high res motion blur getting blended at game start on one of your branches, which was also fixed, I don't perceive fix-worthy delays at game start, so occasionally cleaning up objects is ok with me. So for now that's a NACK.

Went back and re-measured this against current master with a scratch timing harness (same "three matches per map" scenario, repeated match start/teardown) rather than resting on the numbers already in the PR description. Two things came out of that:

The underlying defect is real and still unfixed on master. GameGUI::init()/~GameGUI() still call Sprite::setHighResolution() unconditionally on every match start and end, and that unconditionally rebuilds the terrain/resource HD atlas from PNG every single time, with no check for "value didn't actually change." Since highResolutionArtwork defaults to true, every player pays this on every custom game, and it doesn't amortize — the cost is flat across repeats, not shrinking.

But the PR's headline number doesn't hold up. Measured against what master actually ships (data/highres/v1, terrain+resources only):

So the real, always-on tax is about 130ms per match, not the "3.28 seconds" quoted in the description — that figure came from testing against the much larger 32-frame HD asset revision that's part of #201, not what's actually on master today. Mixing those two scales made the case sound stronger than the evidence for master alone supports, and I think that's a fair part of why this didn't land for you.

Given the corrected number, I agree with the NACK — a real but modest inefficiency doesn't justify this PR's size (lifecycle rework, new test harnesses) on its own right now. Closing it. If the larger pack from #201 ships, the same defect gets a lot more expensive (that's where the 3.28s number actually came from), so I'll revisit it there if it turns out to matter in practice rather than resurrecting this PR speculatively.

Also, noted on the pace — sorry for the rapid-fire pushes/pings, will slow down.